🛡️ Sentinel: [CRITICAL] Fix command injection and secret leak in github-clone#33
🛡️ Sentinel: [CRITICAL] Fix command injection and secret leak in github-clone#33bobdivx wants to merge 1 commit into
Conversation
…ub-clone Co-authored-by: bobdivx <6737167+bobdivx@users.noreply.github.com>
|
đź‘‹ Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a đź‘€ emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the security of the GitHub cloning API by transitioning from exec to execFile to mitigate command injection risks and implementing token redaction in error responses. It also includes documentation on preventing secret leakage. Feedback highlights the need for more robust validation of repoName to prevent path traversal and recommends returning generic error messages to the client to avoid information exposure, even when sanitized.
| if (repoUrl.startsWith('-') || repoName.startsWith('-')) { | ||
| return new Response(JSON.stringify({ error: 'Les paramètres ne peuvent pas commencer par un tiret' }), { status: 400 }); |
There was a problem hiding this comment.
The current validation only checks for leading hyphens to prevent flag injection. However, repoName is used in a path.join operation and as a directory argument for git clone, making it susceptible to path traversal attacks (e.g., repoName being ../../etc). Additionally, since these values come from a JSON request, their types should be verified to ensure they are strings before calling .startsWith() to avoid potential runtime errors.
const isSafeName = typeof repoName === 'string' && /^[a-zA-Z0-9._-]{1,128}$/.test(repoName) && repoName !== '..' && repoName !== '.';
if (typeof repoUrl !== 'string' || repoUrl.startsWith('-') || !isSafeName) {
return new Response(JSON.stringify({ error: 'Paramètres invalides ou nom de dépôt non sécurisé' }), { status: 400 });| return new Response(JSON.stringify({ error: error.message || 'Erreur lors du clonage' }), { | ||
| // 🛡️ Security: Sanitize error message to prevent leaking GitHub token to the client | ||
| // since Node.js exec/execFile errors include the command executed | ||
| const safeErrorMsg = (error.message || 'Erreur lors du clonage').replace(/oauth2:[^@]+@/g, 'oauth2:***@'); |
There was a problem hiding this comment.
While redacting the token from the error message is a necessary step when using execFile, returning the raw error message to the client is still a security risk (Information Exposure). It can reveal internal paths, environment details, or software versions. It is recommended to log the full error on the server and return a generic error message to the user, as also suggested in your .jules/sentinel.md file.
// Log the error server-side for debugging
console.error('[github-clone] Error:', error);
const safeErrorMsg = 'Erreur lors du clonage du dépôt. Veuillez vérifier l\'URL et le nom du dépôt.';1 participant
🚨 Severity: CRITICAL
đź’ˇ Vulnerability:
/api/github-cloneendpoint executed thegit clonecommand usingexecwith string interpolation, exposing the application to command injection via therepoUrlandrepoNameparameters.-, leading to flag injection.execerror object included the full command string, including theoauth2token. This error message was passed back to the client directly, leaking the GitHub token.🎯 Impact:
🔧 Fix:
exectoexecFileusing argument arrays, entirely bypassing the shell and mitigating command injection.-) at the start of parameters to prevent flag injection.oauth2token before returning the 500 error response to the client..jules/sentinel.mdoutlining the learning thatchild_processerror messages inherently leak arguments, necessitating redaction when secrets are passed.âś… Verification:
pnpm run checkandpnpm testsuccessfully.PR created automatically by Jules for task 10633150506117268059 started by @bobdivx